fix(infer): normalize parameter shorthand before batching - #5857
fix(infer): normalize parameter shorthand before batching#5857njzjz-bot wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeepEval gains shared validation and expansion for ChangesParameter shorthand normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DeepEval
participant Standardizer
participant Backend
Caller->>DeepEval: provide fparam and aparam
DeepEval->>Standardizer: pass frame, atom, and model dimensions
Standardizer-->>DeepEval: return standardized parameters
DeepEval->>Backend: evaluate normalized inputs
Backend-->>Caller: return evaluation outputs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deepmd/pt/infer/deep_eval.py`:
- Around line 548-555: Update eval_embedding to call _standardize_fparam_aparam
on fparam and aparam before dispatching to self._eval_func, using the embedding
evaluation’s frame count, natoms, and model parameter dimensions. Pass the
standardized values to the backend so shorthand inputs are batched by frames
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2d24567-276c-4ec1-a33e-25ee33258076
📒 Files selected for processing (7)
deepmd/infer/deep_eval.pydeepmd/jax/infer/deep_eval.pydeepmd/pd/infer/deep_eval.pydeepmd/pt/infer/deep_eval.pydeepmd/tf2/infer/deep_eval.pysource/tests/common/test_deep_eval_parameter_shorthand.pysource/tests/consistent/io/test_io.py
Standardize frame and atomic parameter shorthand in the common wrapper and backend entry points before automatic batching. Preserve full per-frame and per-atom arrays while broadcasting documented shared forms consistently across TensorFlow 2, PyTorch, JAX, and Paddle. Normalize PyTorch embedding extraction before split execution so eval_embedding, eval_descriptor, and eval_fitting_last_layer accept shared fparam, per-atom aparam, and scalar aparam forms. Cover two frames forced through one-frame backend batches. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
2149d24 to
9227907
Compare
for more information, see https://pre-commit.ci
|
Possible reviewers based on changed lines, exact file history, and exact-file review history:
No review request was made automatically. Coding agent: Codex |
…shorthand-backends-5666
deserialize_to_file only writes .json, serialize_from_file raises NotImplementedError, and the .json reader rejects fparam/aparam, so the paddle entry could not complete the round trip. Its normalization stays covered by test_deep_eval_parameter_shorthand.py.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/tests/consistent/io/test_io.py (1)
264-267: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse non-uniform parameter values in this regression test.
Because all parameters are filled with ones, incorrect frame/atom/feature tiling or axis permutation can still produce identical outputs. Use distinct values so the test validates normalization semantics, not just shapes.
Proposed test data fix
- fparam_shared = np.ones(deep_eval.get_dim_fparam()) - aparam_per_atom = np.ones((natoms, deep_eval.get_dim_aparam())) + fparam_shared = np.arange( + 1, deep_eval.get_dim_fparam() + 1, dtype=GLOBAL_NP_FLOAT_PRECISION + ) + aparam_per_atom = np.arange( + 1, + natoms * deep_eval.get_dim_aparam() + 1, + dtype=GLOBAL_NP_FLOAT_PRECISION, + ).reshape(natoms, deep_eval.get_dim_aparam()) ... - np.ones(deep_eval.get_dim_aparam()), + np.arange( + 1, + deep_eval.get_dim_aparam() + 1, + dtype=GLOBAL_NP_FLOAT_PRECISION, + ),Also applies to: 278-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/consistent/io/test_io.py` around lines 264 - 267, Update the regression test data around fparam_shared, aparam_per_atom, fparam_full, and aparam_full to use distinct, non-uniform values across frames, atoms, and features; apply the same change to the corresponding setup at the additional referenced location so incorrect tiling or axis permutation produces different outputs and the test validates normalization semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@source/tests/consistent/io/test_io.py`:
- Around line 264-267: Update the regression test data around fparam_shared,
aparam_per_atom, fparam_full, and aparam_full to use distinct, non-uniform
values across frames, atoms, and features; apply the same change to the
corresponding setup at the additional referenced location so incorrect tiling or
axis permutation produces different outputs and the test validates normalization
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8490894b-d101-4255-a490-f334eda1c482
📒 Files selected for processing (2)
deepmd/infer/deep_eval.pysource/tests/consistent/io/test_io.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/infer/deep_eval.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5857 +/- ##
==========================================
+ Coverage 79.03% 79.24% +0.20%
==========================================
Files 1055 1072 +17
Lines 122233 125055 +2822
Branches 4401 4541 +140
==========================================
+ Hits 96607 99098 +2491
- Misses 24061 24332 +271
- Partials 1565 1625 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The core change is right, and putting the ladder in one shared helper called before AutoBatchSize is the correct design. I checked it rather than assuming: full-shape inputs are unchanged across a wide sweep of dim_fparam / dim_aparam / nframes / auto_batch_size combinations, the public path stays idempotent (the flattened 2-D form re-enters on the nframes*natoms*dim rung and only reshapes, no double-tiling), the three size-dispatch collision families (nframes==1, natoms==1, both) produce identical arrays in every colliding branch, and the spin route gets the real-atom natoms that _eval_model_spin expects. It also fixes the long-standing "got wrong size of frame param" typo in the aparam branch.
I ran the new test_embedding.py case against the pre-fix tree and it fails with ValueError: cannot reshape array of size 1 into shape (1,7,1), so that one is a genuine regression test - two frames, a forced per-frame split, and np.linspace(0.1, 0.7, natoms) so aparam actually varies per atom. Good test.
My comments are all about coverage and scope, not the helper. Six inline. The two I would weigh most are the ones about what the tests can actually detect: test_io.py uses uniform values, and on a GPU runner neither new test performs the split it is built around.
Note the head moved while I was reviewing (bbfc000ac2b1 -> 8d80b8579); everything below is against 8d80b8579.
Smaller notes, not worth their own threads:
-
The TF v1 backend has the same defect and is worse than the others:
deepmd/tf/infer/deep_eval.pypassesfparam=/aparam=as kwargs intoexecute_all(which slices kwargs too), and its shorthand ladder lives inside_prepare_feed_dict, i.e. per batch, after slicing. With one frame per batch a(natoms, dim_aparam)array is sliced on its atom axis to(1, dim_aparam), which then matches thesize == dim_aparamrung and tiles atom 0's value onto every atom - wrong numbers, no exception. Out of this PR's stated scope, but"tensorflow"is in the same test loop and its exclusion is the undocumented one. -
The PR body says it covers "both normal and spin backend routes". The placement before the spin branch is correct, but no test in the diff constructs a spin model or passes
spin=, so_eval_model_spinis never exercised with shorthand. -
test_embedding.py'sassertEqual(backend.auto_batch_size.current_batch_size, natoms)fails outright ifDP_INFER_BATCH_SIZEis exported, sinceAutoBatchSize.__init__lets that variable override the constructor argument - and if it is set, the test silently stops forcing the split while still passing. -
The helper unit tests never pass
fparamandaparamtogether, which is the shape every production call site uses. The two branches are independent so nothing is likely hiding there. -
_standard_inputbuilds the canonical 3-D aparam and immediately re-flattens it to 2-D. Every consumer reshapes from.sizeregardless of incoming rank, so nothing needs the 2-D form any more; dropping the re-flatten would make the internal boundary unambiguous. -
The aparam error text changes from "frame param" to "atomic param" (a real fix), but
deepmd/tf/infer/deep_eval.pystill carries the old wording in its own duplicate check, so the two entry points now disagree. Worth a line in the PR description since it is user-visible.
Normalize parameter shorthand in the remaining dpmodel, pt_expt, and TensorFlow adapters before automatic batching. Strengthen the regressions with distinct frames and parameters, fresh batchers, and direct Paddle and pt_expt adapter coverage. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Coding-Agent: Codex\nCodex-Version: codex-cli 0.144.6\nModel: gpt-5.6-sol\nReasoning-Effort: xhigh
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thanks -- I went through all six of my earlier threads against HEAD f785c9e0b rather than the replies, and they are genuinely addressed:
- The consistency test no longer compares byte-identical frames. Distinct coordinates (
+0.125), a perturbed box,linspaceparameters and distinct per-frame rows mean a mis-tiling now changes a number instead of coinciding. - The batching half of the rationale is restored: each case builds a fresh
DeepEval(model_file, auto_batch_size=natoms)and issues the shorthand call first, with a separate unbatched evaluator for the reference, so GPU batch-size growth cannot silently remove the split. Relaxing to1e-10for the cross-batch comparison is the right call given differing GEMM shapes. - The allowlist gap is closed the better way -- dpmodel and tensorflow are exercised rather than excluded, and Paddle's absence now carries a stated reason instead of being invisible.
- The pd hunk has executing coverage again, and the PR body is corrected to say plainly that it runs in the Paddle environment rather than locally.
_standard_inputitself now delegates to the helper and re-flattens to the historical 2-D ABI, so the duplication I was worried about is genuinely gone: one definition serving the public wrapper and every adapter.- The helper's numpydoc block covers the parameters and both return shapes, including the 3-D aparam and the flattening note.
I also checked the two things that would worry me about a refactor of this shape. An exhaustive sweep of the new helper against master's _standard_input ladder over nframes x natoms x dim_fparam x dim_aparam x input size x candidate shapes produced no difference in value, shape, or raise behaviour -- the .size dispatch is ambiguous only in the cases where both branches yield the same array, exactly as before. And on the regression side I ran your new pt_expt adapter test against unfixed code: it fails there, so it does prove the fix.
One gap left, inline, on the adapter I asked about last time. I would rather see it closed here than tracked separately, since it is the same contract and the same file.
Two smaller notes, neither needing action in this PR. deepmd/tf/infer/deep_eval.py#L850-L862 still holds the old private ladder; both of its callers now receive normalized input so it is unreachable in practice, but it still carries the copy-paste bug your helper fixes -- the aparam branch raises "got wrong size of frame param". Worth deleting or reducing to an assertion while the context is fresh. And PretrainedDeepEvalBackend (deepmd/pretrained/deep_eval.py#L55-L60) is an eighth DeepEvalBackend subclass not named in the PR body's "seven adapters"; it is a pure delegator so it inherits the fix, but it is worth a mention so nobody assumes it was audited and missed.
Unrelated to the diff: the Read the Docs check is failing on this head. I could not read the build log (the page is not publicly accessible), and #5908 fails it too while other open PRs pass, so it may well be infrastructure rather than yours -- flagging only so it is not overlooked.
…r routes Wire _standardize_fparam_aparam into eval_descriptor and eval_fitting_last_layer so shared per-atom shorthand cannot be mistaken for a frame axis before _prepare_nlist_inputs reshapes it. Extend the pt_expt adapter regressions to cover both routes. Coding-Agent: opencode opencode-Version: 1.18.9 Model: ustc/deepseek-v4-flash Reasoning-Effort: max
The backend-direct shorthand comparison never checked that the two input frames actually differ, so a degenerate expansion that reuses one frame's parameters for every frame would also make the frame-major reference degenerate and slip past the allclose comparison. Assert the per-frame energy values differ on every shorthand case. Coding-Agent: opencode opencode-Version: 1.18.9 Model: ustc/deepseek-v4-flash Reasoning-Effort: max
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
njzjz-bot
left a comment
There was a problem hiding this comment.
Reviewed head 1709902d73dc6dfb92795b0cb956d3716d9c5df0 with three independent full subagent reviews. No actionable code findings were identified. The shared normalization preserves the documented shorthand semantics and historical backend ABI across the reviewed adapters. The PR is currently conflicting, and the Read the Docs and pre-commit checks should be rerun after rebasing. Because this PR was opened by the active njzjz-bot account, this is a comment-only review rather than a self-approval event.
The quota is about to reset, so I am concentrating the remaining token budget on these reviews.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Closes #5666.
Summary
Why existing tests missed this
Existing multi-frame inference tests call the high-level DeepPot.eval wrapper, whose _standard_input already expands shorthand before backend dispatch. Backend-direct consistency tests used canonical full shapes, while many backend-specific fixtures use only one frame, where shorthand sizes can coincide with full sizes.
The regressions deliberately call backend methods directly, use distinct coordinates and parameter values, and invoke shorthand/full inputs first on fresh auto_batch_size=natoms evaluators. This catches axis swaps, frame reuse, and GPU auto-batch growth that could otherwise make the intended split disappear.
Validation
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
New Features
Bug Fixes
Tests